Refactor: Unified Paint Abstraction & Stroke Gradient Support [FEATURE]#3975
Refactor: Unified Paint Abstraction & Stroke Gradient Support [FEATURE]#3975Chetansahney wants to merge 3 commits intoGraphiteEditor:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for gradient strokes by adding a gradient field to the Stroke struct and updating the rendering logic for both SVG and Vello-based outputs. The review feedback identifies several improvement opportunities, including refactoring duplicated gradient brush creation into a shared helper function, implementing smoother transitions for gradient interpolation in the lerp method, and standardizing the with_color API return type for consistency. It is also suggested to use unreachable! in branches that are logically covered by prior checks to improve code clarity and error detection.
I am having trouble creating individual review comments. Click here to see my feedback.
node-graph/libraries/rendering/src/render_ext.rs (125)
This branch seems unreachable. The if !self.has_renderable_stroke() check on line 110 should handle the case where both gradient and color are None, causing an early return. Using unreachable!() here would make this assumption explicit and catch potential logic errors in the future if has_renderable_stroke changes.
_ => unreachable!("Stroke should have a color or gradient to be rendered, as checked by has_renderable_stroke"),
node-graph/libraries/rendering/src/renderer.rs (1135-1193)
The logic for creating a gradient brush here is very similar to the logic for fill gradients in the do_fill_path closure (lines 1037-1088). To improve maintainability and reduce code duplication, consider extracting this gradient-to-brush conversion logic into a shared helper function. This function could take a &Gradient and the necessary transforms, and return a (peniko::Brush, Option<kurbo::Affine>) tuple.
node-graph/libraries/vector-types/src/vector/style.rs (364-369)
The interpolation logic for gradients here uses a hard switch at time = 0.5 when one of the strokes has a gradient and the other doesn't. This will result in an abrupt visual change rather than a smooth transition.
For a better user experience, consider implementing a smoother interpolation, for example by fading the gradient's alpha to transparent. You could look at Fill::lerp for inspiration on how to handle transitions between different paint types (e.g., solid color to gradient).
node-graph/libraries/vector-types/src/vector/style.rs (433-440)
The function with_color returns an Option<Self> but always returns Some(self). For consistency with with_gradient and with_weight which return Self, and to simplify the API, consider changing the return type to Self.
pub fn with_color(mut self, color: &Option<Color>) -> Self {
self.color = *color;
if color.is_some() {
self.gradient = None;
}
self
}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
Confidence score: 3/5
- There is a concrete rendering-risk issue in
node-graph/libraries/vector-types/src/vector/style.rs:has_renderable_stroke()currently ORs gradient/color alpha checks, which can incorrectly mark a transparent gradient stroke as renderable due to ignored color state. - The new stroke-gradient path in
node-graph/libraries/rendering/src/renderer.rsduplicates complex fill-gradient transform/construction logic; this is not an immediate break but raises medium regression risk from future fill/stroke divergence. - The PR title policy violation in
node-graph/libraries/rendering/src/renderer.rsappears process-related rather than runtime-impacting, so it lowers confidence only slightly compared with the rendering behavior concerns. - Pay close attention to
node-graph/libraries/vector-types/src/vector/style.rsandnode-graph/libraries/rendering/src/renderer.rs- stroke renderability precedence and duplicated gradient logic could cause visible rendering inconsistencies.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="node-graph/libraries/rendering/src/renderer.rs">
<violation number="1" location="node-graph/libraries/rendering/src/renderer.rs:1197">
P1: Custom agent: **PR title enforcement**
PR title format violates the title policy: it uses a colon-prefixed `Refactor:` tag and title case wording instead of a sentence-case imperative title.</violation>
<violation number="2" location="node-graph/libraries/rendering/src/renderer.rs:1221">
P2: New stroke-gradient code duplicates complex fill-gradient transform/construction logic, increasing risk of fill/stroke rendering divergence.</violation>
</file>
<file name="node-graph/libraries/vector-types/src/vector/style.rs">
<violation number="1" location="node-graph/libraries/vector-types/src/vector/style.rs:523">
P2: `has_renderable_stroke()` violates gradient-over-color precedence by OR-ing both alpha checks, so an ignored color can make a transparent gradient stroke appear renderable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -1194,34 +1194,90 @@ impl Render for Table<Vector> { | |||
| }; | |||
There was a problem hiding this comment.
P1: Custom agent: PR title enforcement
PR title format violates the title policy: it uses a colon-prefixed Refactor: tag and title case wording instead of a sentence-case imperative title.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 1197:
<comment>PR title format violates the title policy: it uses a colon-prefixed `Refactor:` tag and title case wording instead of a sentence-case imperative title.</comment>
<file context>
@@ -1194,34 +1194,90 @@ impl Render for Table<Vector> {
- None => peniko::Color::TRANSPARENT,
- };
- let cap = match stroke.cap {
+ if let Some(stroke_style) = row.element.style.stroke() {
+ let cap = match stroke_style.cap {
StrokeCap::Butt => Cap::Butt,
</file context>
| let has_color_alpha = self.color.is_some_and(|color| color.a() != 0.); | ||
| let has_gradient_alpha = self.gradient.as_ref().is_some_and(|gradient| gradient.stops.color.iter().any(|color| color.a() != 0.)); | ||
|
|
||
| has_color_alpha || has_gradient_alpha |
There was a problem hiding this comment.
P2: has_renderable_stroke() violates gradient-over-color precedence by OR-ing both alpha checks, so an ignored color can make a transparent gradient stroke appear renderable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/vector-types/src/vector/style.rs, line 523:
<comment>`has_renderable_stroke()` violates gradient-over-color precedence by OR-ing both alpha checks, so an ignored color can make a transparent gradient stroke appear renderable.</comment>
<file context>
@@ -488,7 +513,14 @@ impl Stroke {
+ let has_color_alpha = self.color.is_some_and(|color| color.a() != 0.);
+ let has_gradient_alpha = self.gradient.as_ref().is_some_and(|gradient| gradient.stops.color.iter().any(|color| color.a() != 0.));
+
+ has_color_alpha || has_gradient_alpha
}
}
</file context>
| has_color_alpha || has_gradient_alpha | |
| if self.gradient.is_some() { has_gradient_alpha } else { has_color_alpha } |
| scene.stroke(&stroke, kurbo::Affine::new(element_transform.to_cols_array()), color, None, &path); | ||
| if kurbo_stroke.width > 0. { | ||
| let (brush, brush_transform) = if let Some(gradient) = stroke_style.gradient.as_ref() { | ||
| let mut stops = peniko::ColorStops::new(); |
There was a problem hiding this comment.
P2: New stroke-gradient code duplicates complex fill-gradient transform/construction logic, increasing risk of fill/stroke rendering divergence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/rendering/src/renderer.rs, line 1221:
<comment>New stroke-gradient code duplicates complex fill-gradient transform/construction logic, increasing risk of fill/stroke rendering divergence.</comment>
<file context>
@@ -1194,34 +1194,90 @@ impl Render for Table<Vector> {
- scene.stroke(&stroke, kurbo::Affine::new(element_transform.to_cols_array()), color, None, &path);
+ if kurbo_stroke.width > 0. {
+ let (brush, brush_transform) = if let Some(gradient) = stroke_style.gradient.as_ref() {
+ let mut stops = peniko::ColorStops::new();
+ for (position, color, _) in gradient.stops.interpolated_samples() {
+ stops.push(peniko::ColorStop {
</file context>
1 participant
This PR initiates the architectural refactor for the "Generalized Graphical Data Rendering" project. It replaces the legacy, color-only stroke model with an extensible system capable of rendering gradients. This serves as the foundational "plumbing" to eventually support patterns, image fills, and canvas-spanning effects across all vector attributes.